Skip to content

Conversation

@duhnnie
Copy link

@duhnnie duhnnie commented Nov 29, 2025

PR Description

This PR resolves two issues related to the ErrorHandler assignment.

Issue 1

There was a problem with how mounted sub-apps were compared against the current request path to determine the appropriate ErrorHandler. Previously, the code iterated directly over app.mountFields.appList (a map). Since Go does not guarantee map iteration order, this resulted in inconsistent behavior: the selected ErrorHandler could differ between runs.

The fix ensures iteration uses app.mountFields.appListKey, an array that contains the mount keys in a deterministic order (sorted by key length for Render). This guarantees consistent iteration and ensures the correct access order into app.mountFields.appList.

Example

Consider the following mounts (sub-apps):

  • [empty]: the root path, with no custom ErrorHandler
  • /api/v1: with custom ErrorHandler
  • /api/v1/users: with no custom ErrorHandler — it should fall back to the nearest upper mount (/api/v1)

The issue arose from iterating over a map containing the path mounts as keys. In Go, map iteration order is nondeterministic.

Suppose a request to /api/v1/users fails, and Fiber attempts to determine the correct ErrorHandler by invoking ErrorHandler(). The map might be iterated in the following order:

  1. [empty]
  2. /api/v1/users
  3. /api/v1 (which has a custom ErrorHandler)

The algorithm contains a condition to select a custom ErrorHandler from the current iteration if applicable. It also checks that the number of path segments is greater than or equal to the previous iteration. In this scenario, the loop stops after the second iteration (/api/v1/users). Because no custom ErrorHandler was found up to that point, Fiber incorrectly falls back to the DefaultErrorHandler, instead of the one in /api/v1.

About the Fix

The fix replaces map iteration with iteration over an existing ordered (by length) array of mount paths.

Important: this bug occurred randomly due to nondeterministic map iteration.

Issue 2

This issue was similar, also related to incorrect path matching. Consider the following mounts:

  • [empty]: no custom ErrorHandler
  • /api/v1: with custom ErrorHandler
  • /api/v1/users: no custom ErrorHandler
  • /api/v1/use: with custom ErrorHandler

Given a failing request to /api/v1/users, and assuming Issue 1 is fixed, the ErrorHandler resolution iterates over the mount paths in this order:

  1. [empty]: empty path, so continue
  2. /api/v1: matches as a prefix and has a custom ErrorHandler, so it is selected
  3. /api/v1/use: incorrectly matches as a prefix and also has a custom ErrorHandler, so it overrides the previous selection
  4. /api/v1/users: matches as a prefix but has no custom ErrorHandler, so it keeps the previous selection

As a result, /api/v1/users ends up using the ErrorHandler from /api/v1/use, which is incorrect. It should use the one from /api/v1.

This happens because a comparison like:

/api/v1/users hasPrefix /api/v1/use → true

is logically incorrect for this use case.

About the Fix

The fix applies normalization to paths only during comparison. The normalization consists of adding a trailing slash to both paths, ensuring comparisons behave correctly:

/api/v1/users/ hasPrefix /api/v1/use/ → false

This guarantees proper matching without affecting the original mounts.

Fixes #3906

@duhnnie duhnnie requested a review from a team as a code owner November 29, 2025 03:55
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 29, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Replaced non-deterministic mount iteration with ordered iteration over app.mountFields.appListKeys, added trailing-slash normalization for path/prefix comparisons via utils.AddTrailingSlash, and added a test ensuring the most-specific mounted sub-app ErrorHandler is chosen.

Changes

Cohort / File(s) Summary
Deterministic mount traversal & path normalization
app.go
Iterate app.mountFields.appListKeys (deterministic order), look up mounted sub-apps by key, compute normalizedPath and normalizedPrefix using utils.AddTrailingSlash for prefix comparisons, and use normalized values to select the matching mount while retaining the original prefix to compute parts.
Tests — error handler selection
app_test.go
Added TestErrorHandler_PicksRightOne which builds nested mounted apps with distinct ErrorHandler callbacks and asserts the correct handler is invoked for routes at different mount depths.
String utility & tests
utils/strings.go, utils/strings_test.go
Added exported AddTrailingSlash(s string) string (uses strings.HasSuffix) and accompanying unit tests and a benchmark covering empty, root, single/multi-level, and long-path cases.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant MainApp
  participant MountResolver as MountResolver
  participant SubApp
  participant ErrorHandler as ErrorHandler

  Client->>MainApp: HTTP request (path)
  MainApp->>MountResolver: iterate mountKeys (deterministic)
  MountResolver->>MountResolver: compute normalizedPath (AddTrailingSlash(ctx.Path()))
  MountResolver->>MountResolver: compute normalizedPrefix (AddTrailingSlash(prefix))
  MountResolver-->>MainApp: select best-matching SubApp by normalized prefix
  MainApp->>SubApp: forward request (with original prefix/parts)
  SubApp->>SubApp: handler returns error
  SubApp->>ErrorHandler: invoke SubApp.ErrorHandler (if set)
  alt no SubApp.ErrorHandler
    SubApp->>MainApp: bubble error to nearest ancestor
    MainApp->>ErrorHandler: invoke ancestor ErrorHandler
  end
  ErrorHandler-->>Client: formatted error response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review focus:
    • app.go: correctness of normalization and matching logic for edge cases (empty prefix, /, overlapping/substring prefixes) and interaction with parts computation.
    • app_test.go: coverage and determinism of mounted-error-handler selection.
    • utils/strings.go & tests: behavior on empty and very long strings; benchmark validity.

Possibly related PRs

Suggested reviewers

  • gaby
  • sixcolors
  • efectn

Poem

"I hopped through keys now tidy and straight,
No map left to chance, no handler lost to fate.
A slash here, a slash there, paths line up just right,
Tests clap tiny paws, errors find their light. 🥕"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main bug fix: resolving ErrorHandler invocation issues for mounted sub-apps. It is concise, specific, and directly reflects the primary changes in the PR.
Description check ✅ Passed The PR description is comprehensive and well-structured, detailing both issues (nondeterministic map iteration and incorrect prefix matching), their causes, fixes, and examples. However, it does not follow the template's structured sections (Changes introduced checklist, Type of change, Checklist items, etc.).
Linked Issues check ✅ Passed The PR addresses all coding requirements from issue #3906: fixes nondeterministic ErrorHandler selection by using deterministic appListKey iteration [#3906], fixes incorrect prefix matching with path normalization [#3906], and includes comprehensive unit tests that validate both issues are resolved [#3906].
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the ErrorHandler invocation bugs: app.go fixes the iteration and comparison logic, app_test.go adds regression tests, and utils/strings.go/strings_test.go add the AddTrailingSlash utility required for path normalization. No extraneous changes detected.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 289adbc and 0ab8c3f.

📒 Files selected for processing (2)
  • app_test.go (1 hunks)
  • utils/strings_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

When adding Go tests, always invoke t.Parallel() at the start of each test and subtest to maximize concurrency

Files:

  • utils/strings_test.go
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • utils/strings_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency

Applied to files:

  • utils/strings_test.go
🧬 Code graph analysis (1)
utils/strings_test.go (1)
utils/strings.go (1)
  • AddTrailingSlash (82-88)
🔇 Additional comments (2)
utils/strings_test.go (2)

219-262: LGTM! Parallel execution and test coverage are excellent.

The test properly invokes t.Parallel() in both the main test and each subtest as required by the coding guidelines. The table-driven approach with comprehensive test cases (covering already-slashed paths, missing slashes, empty strings, root paths, and multi-level paths) provides solid coverage for the AddTrailingSlash utility function.


264-280: LGTM! Comprehensive benchmark coverage.

The benchmark effectively covers multiple scenarios including edge cases (empty string) and performance-critical cases (very long strings). The use of a map for test cases introduces non-deterministic ordering in benchmark output, but this does not affect correctness since each sub-benchmark runs independently.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @duhnnie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug in the application's error handling mechanism, specifically concerning how error handlers are invoked for mounted sub-applications. By switching from an unordered map iteration to an ordered key list, it ensures that the correct error handler is consistently identified and used, thereby improving the reliability and predictability of error management across the application.

Highlights

  • Fix Inconsistent ErrorHandler Selection: Resolved an issue where the selection of the appropriate ErrorHandler for mounted sub-applications was inconsistent due to the non-guaranteed iteration order of Go maps.
  • Ensure Consistent Order: The fix involves iterating over app.mountFields.appListKeys (an ordered array of application keys) instead of directly iterating over the app.mountFields.appList map, guaranteeing a consistent order for ErrorHandler selection.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses an issue of non-deterministic behavior when selecting an ErrorHandler for mounted sub-apps. The change correctly replaces iteration over a map with iteration over a pre-sorted slice of keys, ensuring a consistent order. However, my review identified a remaining logical flaw in how the most specific error handler is chosen, which could lead to an incorrect handler being used in certain scenarios. I've left a detailed comment on this.

Comment on lines 1044 to 1052
for _, prefix := range app.mountFields.appListKeys {
subApp := app.mountFields.appList[prefix]
if prefix != "" && strings.HasPrefix(ctx.path, prefix) {
parts := len(strings.Split(prefix, "/"))
if mountedPrefixParts <= parts {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While this change correctly addresses the non-deterministic iteration by using a sorted slice of keys, there's a remaining logical issue in how the error handler is selected. The current logic can incorrectly choose an error handler from a less specific mounted app.

For example, if you have apps mounted at /api (with a custom error handler) and /api/v1 (without one), a request to /api/v1/users will incorrectly use the error handler from /api instead of falling back to the root app's handler.

This happens because mountedErrHandler is not reset when a more specific prefix (with no custom error handler) is found. The logic should be adjusted to ensure that only the error handler for the most specific matching prefix is considered. If that most specific prefix doesn't have a custom error handler, mountedErrHandler should be nil so that the root handler is used.

To fix this, the logic should be updated to reset mountedErrHandler when a more specific prefix is found.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can anyone check if what gemini-code-assist says is correct? I think that if you have apps mounted at /api (with a custom error handler) and /api/v1 (without one), a request to /api/v1/users should use the error handler from the closest mount-path which would be /api instead of falling back to the root app's handler. Am i right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for raising this important question, @duhnnie. I apologize for the confusion in my previous comment; my analysis of the desired behavior and the current code's outcome after your PR was indeed mistaken.

You are absolutely correct in your understanding:

"if you have apps mounted at /api (with a custom error handler) and /api/v1 (without one), a request to /api/v1/users should use the error handler from the closest mount-path which would be /api instead of falling back to the root app's handler."

This is the correct and expected hierarchical fallback behavior for mounted applications.

Let me clarify why your PR, with the change to iterate over app.mountFields.appListKeys (which are sorted by key length, typically ascending), actually achieves this desired behavior:

  1. Iteration Order: By iterating through appListKeys sorted by length (e.g., "/", "/api", "/api/v1"), the loop processes shorter, less specific prefixes first, and then moves to longer, more specific ones.
  2. mountedErrHandler and mountedPrefixParts:
    • When the loop encounters a matching prefix (e.g., "/api" for "/api/v1/users") that has an errorHandler, it updates mountedErrHandler to that handler and mountedPrefixParts to its length.
    • Crucially, when it later encounters a more specific matching prefix (e.g., "/api/v1") that does not have its own errorHandler, mountedErrHandler is not overwritten (because subApp.errorHandler is nil). However, mountedPrefixParts is updated to the length of this more specific prefix (e.g., 2 for "/api/v1").
  3. The mountedPrefixParts <= parts Condition: This condition ensures that once a more specific prefix has been processed (and mountedPrefixParts updated to its length), any less specific prefixes (i.e., those with parts less than the current mountedPrefixParts) will be skipped, preventing them from overwriting the mountedErrHandler with a less specific one, unless the current mountedErrHandler is nil and a shorter prefix has a handler.

Example Trace (with EH_ROOT, EH_A for /api, and nil for /api/v1):

  • Initial: mountedErrHandler = nil, mountedPrefixParts = 0
  • Prefix /: Matches. EH_ROOT is set. mountedPrefixParts = 1.
  • Prefix /api: Matches. EH_A is set (overwriting EH_ROOT). mountedPrefixParts = 1.
  • Prefix /api/v1: Matches. subApp.errorHandler is nil. mountedErrHandler remains EH_A. mountedPrefixParts becomes 2.

Result: After the loop, mountedErrHandler will correctly be EH_A, which is the handler for the closest mounted app (/api) that has one, as you intended.

My apologies again for the misdirection. Your PR correctly implements the desired hierarchical error handler selection. The initial comment was based on an incorrect interpretation of the code's flow with the sorted keys.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical bug where the ErrorHandler selection for mounted sub-apps was non-deterministic due to Go's unordered map iteration. The fix ensures consistent ErrorHandler resolution by iterating over an ordered slice of keys instead of the map directly.

Key Changes:

  • Modified the ErrorHandler method to iterate over app.mountFields.appListKeys (ordered slice) instead of directly iterating the app.mountFields.appList map
  • This guarantees deterministic ErrorHandler selection that matches the longest prefix, resolving issue #3906

@gaby gaby changed the title 🩹 Fix ErrorHandler invokation 🩹 bug: Fix ErrorHandler invocation for mounted sub-apps Nov 29, 2025
@gaby
Copy link
Member

gaby commented Nov 29, 2025

@codex review

@chatgpt-codex-connector
Copy link

Codex Review: Didn't find any major issues. Hooray!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ReneWerner87
Copy link
Member

@duhnnie Thanks for the fix.

Do you know if the problem also exists in version 3?

Can you add a unit test that would fail in the target branch without your fix and would run after your fix?
Then I would also include it in version 3.
This would also ensure that the error no longer sneaks in in special cases in the future.

@ReneWerner87 ReneWerner87 linked an issue Nov 29, 2025 that may be closed by this pull request
3 tasks
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
utils/strings.go (1)

7-8: Document the new exported helper (and optionally simplify the check).

AddTrailingSlash’s behavior looks correct, but it’s exported and currently lacks a Go doc comment, unlike the other helpers in this file. Please add a short comment (// AddTrailingSlash ...) so linters and docs stay clean. You could also (optionally) avoid importing strings by checking the last byte directly:

func AddTrailingSlash(s string) string {
	if len(s) > 0 && s[len(s)-1] == '/' {
		return s
	}
	return s + "/"
}

Also applies to: 79-85

app_test.go (1)

1981-2053: Align the new test with project conventions and clean up leftovers.

The core logic of TestErrorHandler_PicksRightOne looks good and matches the intended error‑handler resolution, but a few small cleanups would help:

  • Add t.Parallel() at the start of TestErrorHandler_PicksRightOne to follow the repo’s convention for new tests.
  • errorHandlerTestCase is currently unused; either wire it into testCases or remove it to avoid dead code.
  • The comment above the "/api/v1/use" case still says "/api/v1/users mount has a custom ErrorHandler"; updating it to "/api/v1/use" would avoid confusion.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9982fb and d754819.

📒 Files selected for processing (3)
  • app.go (1 hunks)
  • app_test.go (1 hunks)
  • utils/strings.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • utils/strings.go
  • app_test.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

When adding Go tests, always invoke t.Parallel() at the start of each test and subtest to maximize concurrency

Files:

  • app_test.go
🧠 Learnings (4)
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Applied to files:

  • utils/strings.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • app_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.

Applied to files:

  • app_test.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • app_test.go

@duhnnie
Copy link
Author

duhnnie commented Nov 30, 2025

@ReneWerner87

I verified that this issue also occurs in Version 3. You can reproduce it by cloning and running this repository: https://github.com/duhnnie/Fiber-bug-repo/tree/V3 (branch V3).

I also identified an additional related issue, which has been fixed as well. Please see the updated PR description above for full details (I’m surprised it went unnoticed until now).

A unit test has been added to cover this fix.

Let me know if you need anything else to proceed with the merge.

Thank you.

@ReneWerner87
Copy link
Member

@duhnnie pls fix the lint errors
image

can you also port this fix to v3 ?

@ReneWerner87
Copy link
Member

I tested it and you have a valid point.

@duhnnie duhnnie force-pushed the v2-fix-custom-errorhandler-invokation branch 2 times, most recently from daa8c85 to 4751ea6 Compare November 30, 2025 14:46
@duhnnie duhnnie force-pushed the v2-fix-custom-errorhandler-invokation branch 2 times, most recently from 030a73f to 8c6b0c0 Compare November 30, 2025 17:40
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c6b0c0 and 3196226.

📒 Files selected for processing (1)
  • utils/strings_test.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

When adding Go tests, always invoke t.Parallel() at the start of each test and subtest to maximize concurrency

Files:

  • utils/strings_test.go
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • utils/strings_test.go
🧬 Code graph analysis (1)
utils/strings_test.go (1)
utils/strings.go (1)
  • AddTrailingSlash (80-86)
🔇 Additional comments (1)
utils/strings_test.go (1)

262-278: LGTM! Comprehensive benchmark coverage.

The benchmark effectively covers various scenarios including edge cases (empty string) and performance edge cases (long strings). The test cases validate both the fast path (already has slash) and slow path (concatenation required).

@duhnnie
Copy link
Author

duhnnie commented Nov 30, 2025

@ReneWerner87 @gaby Ok guys, I think I have this PR ready.

  • lint issues addressed ✅
  • unit tests added (for fix implementation and new AddTrailingSlash() util function) ✅

Respecting Version 3, I need this PR to be merged first in order to port this fix to V3: gofiber/utils#162

@duhnnie duhnnie force-pushed the v2-fix-custom-errorhandler-invokation branch from 3196226 to 2c70b2e Compare November 30, 2025 22:09
@ReneWerner87
Copy link
Member

@duhnnie pls check lint
image

@duhnnie duhnnie force-pushed the v2-fix-custom-errorhandler-invokation branch from 2c70b2e to 289adbc Compare December 7, 2025 23:39
@duhnnie
Copy link
Author

duhnnie commented Dec 7, 2025

@ReneWerner87 Done, I fixed the linting issue.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c70b2e and 289adbc.

📒 Files selected for processing (4)
  • app.go (1 hunks)
  • app_test.go (1 hunks)
  • utils/strings.go (2 hunks)
  • utils/strings_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • utils/strings.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • app.go
  • app_test.go
  • utils/strings_test.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

When adding Go tests, always invoke t.Parallel() at the start of each test and subtest to maximize concurrency

Files:

  • app_test.go
  • utils/strings_test.go
🧠 Learnings (5)
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.

Applied to files:

  • app_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • app_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.

Applied to files:

  • app_test.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • app_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency

Applied to files:

  • utils/strings_test.go
🧬 Code graph analysis (2)
app.go (1)
utils/strings.go (1)
  • AddTrailingSlash (82-88)
utils/strings_test.go (1)
utils/strings.go (1)
  • AddTrailingSlash (82-88)
🔇 Additional comments (3)
app.go (1)

1044-1060: LGTM! Correctly fixes both nondeterministic iteration and prefix matching issues.

The changes properly address:

  1. Deterministic iteration via appListKeys (sorted slice) instead of direct map iteration
  2. Trailing-slash normalization prevents false-positive prefix matches (e.g., /api/v1/users/ no longer matches /api/v1/use/)

The hierarchical fallback logic is preserved: when a more specific prefix matches but has no custom ErrorHandler, the previously found handler from a less specific prefix remains in mountedErrHandler.

utils/strings_test.go (2)

219-262: LGTM! Well-structured table-driven test with proper parallelization.

The test correctly invokes t.Parallel() at both the top level and within each subtest, as required by the coding guidelines. Test cases cover the essential edge cases including empty strings, root slash, and multi-level paths.


264-280: Good benchmark coverage.

The benchmark includes useful cases for performance analysis, including long strings (10,000 chars) that help identify potential allocation overhead from string concatenation.

@duhnnie duhnnie force-pushed the v2-fix-custom-errorhandler-invokation branch from 289adbc to 0ab8c3f Compare December 8, 2025 01:10
@ReneWerner87 ReneWerner87 merged commit 4ff945a into gofiber:v2 Dec 8, 2025
27 checks passed
@welcome
Copy link

welcome bot commented Dec 8, 2025

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

@duhnnie pls add the same fix for v3

@gaby
Copy link
Member

gaby commented Dec 8, 2025

@duhnnie pls add the same fix for v3

For v3 is has to use the new helper in utils

@duhnnie
Copy link
Author

duhnnie commented Dec 8, 2025

Port for V3: #3930

@ReneWerner87
Copy link
Member

Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 [Bug]: Custom ErrorHandler is not always called

3 participants